-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ccip-3398 populate state initial PR #14391
Conversation
…t/chainlink into ccip-3398-populate-state
…t/chainlink into ccip-3398-populate-state
@@ -219,7 +213,7 @@ func LoadChainState(chain deployment.Chain, addresses map[string]deployment.Type | |||
if err != nil { | |||
return state, err | |||
} | |||
state.RMNRemote = rmnRemote | |||
state.RMNRemote = rmn1_6.New(rmnRemote) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come rmn1_6
if this case statement is pointing to Version1_0_0
?
case deployment.NewTypeAndVersion(RMNRemote, deployment.Version1_0_0...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, I will update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ogtownsend can you point me to the code ref ? I see that TypeAndVersion for RMN is RMNRemote 1.6.0-dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, @connorwstein I think the case statement should be changed to Version1_6_0_dev
, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah should be changed
) | ||
|
||
type Chain struct { | ||
DestinationChainSelectors []uint64 `json:"destinationChainSelectors,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about caching state from specific contracts at this top level - hard to follow where it comes from. Can we keep the state from each contract in the contract specific view so that its explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed DestinationChainSelectors in OnRamp
and feeQuoter
as of now to populate corresponding config. Do you suggest we pass router struct in the corresponding snapshot function to retrieve the chain selectors by calling getOffRamps?
DestinationChainSelectors []uint64 `json:"destinationChainSelectors,omitempty"` | ||
// TODO - populate supportedTokensByDestination | ||
SupportedTokensByDestination map[uint64][]string `json:"supportedTokensByDestination,omitempty"` | ||
TokenAdminRegistry map[string]v1_5.TokenAdminRegistry `json:"tokenAdminRegistry,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting - I wasn't imagining we'd have multiple deployments of the same version of a contract on the same chain at any given time, but I guess its still possible so good to keep that flexibility.
// TODO - populate supportedTokensByDestination | ||
SupportedTokensByDestination map[uint64][]string `json:"supportedTokensByDestination,omitempty"` | ||
TokenAdminRegistry map[string]v1_5.TokenAdminRegistry `json:"tokenAdminRegistry,omitempty"` | ||
FeeQuoter map[string]v1_6.FeeQuoter `json:"feeQuoter,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the json marshalling would still work if we kept keys as common.Address right because they have a String() method? Stronger typing there would prevent unexpected keys like "blah"
@@ -0,0 +1,33 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk if the types directory is helping much - think we could just have
view/chain.go
view/v1_2/..
view/v1_5/...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type directory is to avoid import cycle, We have the Contract struct getting imported into v1_2 ,v1_5 etc from parent view and then Chain struct has elements from v1_2, v1_5 in the parent view package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I think it could be on the same hierarchical level though just to avoid the nesting
Address string `json:"address,omitempty"` | ||
} | ||
|
||
type ContractState interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused interface?
types.Contract | ||
WrappedNative string `json:"wrappedNative,omitempty"` | ||
ARMProxy string `json:"armProxy,omitempty"` | ||
OnRamps map[uint64]string `json:"onRamps,omitempty"` // Map of DestinationChainSelectors to OnRamp Addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the views are supposed to be human readable, I think we want to resolve the chain selectors into chain names here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chain name derived from chain-selectors repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on second thought, let's think more about how to do this in a uniform way for both selectors and addresses. Can leave it like this for now
} | ||
|
||
func TokenAdminRegistrySnapshot(taContract *token_admin_registry.TokenAdminRegistry) (TokenAdminRegistry, error) { | ||
tokens, err := taContract.GetAllConfiguredTokens(nil, 0, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be an interesting problem here, there could be an unbounded number of tokens since its permissionless. We can start by just listing them all but lets add a TODO here for smarter pruning/DoS protection later.
onRamps[selector] = onRamp.Hex() | ||
} | ||
return Router{ | ||
Contract: types.Contract{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Owner is a pretty widespread field we can include that in the Contract type too I think
if err != nil { | ||
return TokenAdminRegistry{}, err | ||
} | ||
return TokenAdminRegistry{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely want to display the registry modules too. Sadly we have to filter events for that
TokenDecimals uint8 `json:"tokenDecimals,omitempty"` | ||
} | ||
|
||
func FeeQuoterSnapshot(fqContract *fee_quoter.FeeQuoter, tokensByDestination map[uint64][]string) (FeeQuoter, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer just only gethwrapper arguments, makes it explicit which contracts the view has dependencies on
} | ||
|
||
func NewContractMetaData(tv Meta, addr common.Address) (ContractMetaData, error) { | ||
tvStr, err := tv.TypeAndVersion(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these might not always exist (e.g. WETH) but we can come back to that later, can just be empty in that case
@@ -49,6 +54,63 @@ type CCIPChainState struct { | |||
TestRouter *router.Router | |||
} | |||
|
|||
func (c CCIPChainState) GenerateView() (view.ChainView, error) { | |||
chainView := view.NewChain() | |||
r := c.Router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultranit - we can avoid these variables and just use c.Router then if c.Router != nil {c.Router.Address()}
Quality Gate passedIssues Measures |
Adds snapshot creation for a set of contracts. The state snapshot can be updated later based on what more fields are required for each one. This is just to set the standard approach to be followed for state extraction.
Rest of the contracts will be covered on subsequent PRs.